-
-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pedantic ghcide #3751
Pedantic ghcide #3751
Conversation
…ticked-promoted-constructors
ccd80e8
to
a9c9e2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff, the CPP is way better. It seems to me that you've come up with a policy for how to write the big blocks of CPP in imports - perhaps you could write it down in a Note somewhere and refer to it from the files where we do this a lot? That would help us keep to it in future.
@@ -111,6 +110,10 @@ import HieDb.Utils | |||
import qualified System.Random as Random | |||
import System.Random (RandomGen) | |||
|
|||
#if !MIN_VERSION_ghc(9,4,0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised we had to add this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was necessary to clean up redundant imports (it's not used GHC >= 9.4 (Or it's possibly rexported by someone else for those versions?!))
|
||
#if MIN_VERSION_ghc(9,0,1) | ||
import GHC.Tc.Gen.Splice | ||
#endif | ||
|
||
#if MIN_VERSION_ghc(9,0,1) && !MIN_VERSION_ghc(9,2,1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In cases like this where it really is multiple ways of getting the same names, I think it can be nice to structure it with #else
s? Not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe else
CPP is not worth it in the imports section. As this statement uses an &&
symbol, it would need to nested CPP, and then you're pretty much back to where we started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good, just one thing I don't think is an improvement
This is taking much more time than I had anticipated, so I'm going get this merged in, and then if I have time later, finish the rest then
Currently the following warnings are fixed
These warnings don't make sense to fix
These warnings are currently left to fix (some of these also might not make sense to fix)
I have also drastically simplified the CPP in the imports section. Whereas before there were several layers of nested if-else CPP flags spread throughout the imports, now there are very clear single-layer if clauses at the end of the standard imports that are ordered by the ghc version they are for.